Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Instrument await using await-tree #682

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: Instrument await using await-tree #682

wants to merge 2 commits into from
+75 −16

Conversation

Harsh1s
Copy link

@Harsh1s Harsh1s commented Mar 8, 2024 •
edited
Loading

  • what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)
    -> FIxes [Feature]: Instrument the execution path of the command (i.e. fast_path or slow_path) #580
    -> Currently, some simple information is logged to display the execution path of the command. This uses more detailed information to show this process (i.e. use await-tree to indicate the duration of each await.).

  • what changes does this pull request make?
    -> Makes use of instrument_await within functions annotated with the #[instrument] macro to add more detailed async tracing.

  • are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)
    -> No

@codecov Codecov
Copy link

codecov bot commented Mar 8, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 75.71429% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 74.59%. Comparing base (e35b35a) to head (16ae376).
Report is 70 commits behind head on master.

❗ Current head 16ae376 differs from pull request most recent head 86ea780. Consider uploading reports for the commit 86ea780 to get more accurate results

Files Patch % Lines
crates/curp/src/server/mod.rs 69.44% 9 Missing and 2 partials ⚠️
crates/xline/src/server/kv_server.rs 33.33% 5 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #682      +/-   ##
==========================================
- Coverage   75.55%   74.59%   -0.96%     
==========================================
  Files         180      172       -8     
  Lines       26938    25342    -1596     
  Branches    26938    25342    -1596     
==========================================
- Hits        20353    18904    -1449     
+ Misses       5366     5270      -96     
+ Partials     1219     1168      -51

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Phoenix500526
Phoenix500526 reviewed Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@Phoenix500526 Phoenix500526 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this modification affect performance in any way? I suggest adding a switch here so that we can enable it as needed.

@Harsh1s
Copy link
Author

Harsh1s commented Mar 12, 2024

Will this modification affect performance in any way? I suggest adding a switch here so that we can enable it as needed.

Sure, I'll make the changes and let you know. Thanks!

@iGxnon
Copy link
Contributor

iGxnon commented Mar 12, 2024

Can you provide an example after running it?

GFX9
GFX9 previously approved these changes Mar 28, 2024
View reviewed changes
@GFX9 GFX9 self-requested a review March 29, 2024 02:01
@GFX9 GFX9 dismissed their stale review March 29, 2024 02:02

There are no relevant performance tests.

@mergify Mergify
Copy link

mergify bot commented Apr 18, 2024

@Harsh1s Your PR is in conflict and cannot be merged.

@mergify mergify bot requested a review from a team April 19, 2024 04:36
@Phoenix500526
Copy link
Collaborator

Hi, @Harsh1s ! This pr has been stalled for 3 weeks. Would you like to update it? 😄

@Harsh1s
Copy link
Author

Harsh1s commented Apr 27, 2024

Hi, @Harsh1s ! This pr has been stalled for 3 weeks. Would you like to update it? 😄

Oh I'm really sorry about the stalling, it's been a busy past month at my uni, a lot of tests and project deadlines. My end semester exams are going on currently too. I'll try to update it within a week for sure. Sorry again!

rogercloud
rogercloud reviewed May 18, 2024
View reviewed changes
crates/curp/Cargo.toml
@@ -14,6 +14,7 @@ version = "0.1.0"
[dependencies]
async-stream = "0.3.4"
async-trait = "0.1.80"
await-tree = "0.1.2"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest version is "0.2.1", would you please update it?

crates/xline/Cargo.toml
@@ -15,6 +15,7 @@ categories = ["KV"]
anyhow = "1.0.82"
async-stream = "0.3.5"
async-trait = "0.1.80"
await-tree = "0.1.2"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above.

@mergify mergify bot requested a review from a team May 18, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rogercloud rogercloud rogercloud left review comments

@Phoenix500526 Phoenix500526 Phoenix500526 left review comments

@GFX9 GFX9 Awaiting requested review from GFX9

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Instrument the execution path of the command (i.e. fast_path or slow_path)
5 participants
@Harsh1s @iGxnon @Phoenix500526 @rogercloud @GFX9